Fix #4404: In LambdaLift, modify flags properly #4804

Medowhill · 2018-07-18T13:17:54Z

No description provided.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2018-07-18T15:20:07Z

compiler/src/dotty/tools/dotc/transform/LambdaLift.scala

@@ -338,11 +338,16 @@ object LambdaLift {
            else (topClass, JavaStatic)
          }
          else (lOwner, EmptyFlags)
+        // drop Module because class is no longer a singleton in the lifted context.
+        val nFlags = local.flags &~ Module | Lifted | maybeStatic
+        // keep Public when it is a local class


Why do local classes need to be public and classes lifted into traits non-final?

In the issue #4404, @mbloms said:

Anonymous classes are compiled without the public final.

Therefore, I tried to keep anonymous classes as public after lifting. However, I did not consider non-anonymous local classes. I think non-anonymous local classes should not be public. I'll correct the code to distinguish anonymous and non-anonymous classes.

At the first, I changed the code simply to keep final always. Then, test t4565_1 failed. It was compiled but during the execution, there was an exception:
Exception in thread "main" java.lang.ClassFormatError: Method A$1 in class T has illegal modifiers: 0x1A
According to the JVM specification, a method declared inside interface cannot have the final flag, so that I thought that final should be dropped when a method is lifted into a trait.

I think leaving the final flag for anonymous/local classes makes sense, but they shouldn't be public: they should never be called by external code.

According to the JVM specification, a method declared inside interface cannot have the final flag,

I see! Can you add this information as a comment in the code? (With a link to the relevant part of the spec)

smarter · 2018-07-18T18:38:28Z

Otherwise, this looks great but is missing tests. You could either add a file in tests/run that uses Java reflection to check flags, or add tests in https://github.com/lampepfl/dotty/blob/master/compiler/test/dotty/tools/backend/jvm/DottyBytecodeTests.scala to check the flags of the emitted bytecode. If you're not sure how to do that, come to http://gitter.im/lampepfl/dotty and we'll help!

Medowhill · 2018-07-19T09:41:14Z

I changed the code to add the private flag always for both methods and classes during the lifting.
I added comments about the JVM specification with its link.
Three tests using the Java reflection are added. The first and second tests are from issue #4404 and the last one is a test to check whether the final flag of a method is dropped correctly when it is lifted into a trait.

allanrenucci · 2018-07-19T11:53:51Z

@Medowhill Can you rebase the PR on master and remove the merge commits?

Medowhill · 2018-07-19T12:18:29Z

I removed the merge commits.

allanrenucci

Otherwise LGTM

allanrenucci · 2018-07-19T12:22:56Z

compiler/src/dotty/tools/dotc/transform/LambdaLift.scala

+        // According to the JVM specification, a method declared inside interface cannot have the final flag.
+        // "Methods of interfaces may have any of the flags in Table 4.6-A set except ACC_PROTECTED, ACC_FINAL, ..."
+        // (https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.6)
+        val initFlags = if (!local.isClass && (newOwner.flags is Trait)) preInitFlags &~ Final else preInitFlags


Why don't you test local.is(Method) instead of !local.isClass?

Personal taste: I think it is clearer to use a var rather than coming up with new unique names. Ie.

var initFlags = ... if (...) initFlags = initFlags &~ Final

I think your suggestion is better than my original code. I'll fix it. Thank you!

allanrenucci · 2018-07-19T12:33:14Z

tests/run/i4404c.scala

+  }
+}
+
+object Test {


Can you explain what you are testing here

trait T { def f = { object O } }

When we compile the above code, the compiler will generate final lazy <accessor> module def O(): O$ inside method f. I tried to check whether the final flag of method O is dropped correctly during the LambdaLift phase through the test.

If you want to test that methods lose their final modifier, I would replace the object by a normal def. I think it is clearer than using an object relying on the fact that it desugars to a lazy val which itself desugars to a method

That would be clearer. I'll change it. Thank you!

allanrenucci · 2018-07-19T16:05:03Z

tests/run/i4404c.scala

+
+trait T {
+  def f = {
+    final def f0 = ()


Can you remove the final modifier since I believe being able to write it in the first place is a bug?

Then, do I need to change the test to use object inside a method?

No I think the test is fine as is. We want to make sure that we don't add the final modifier in this case

allanrenucci

LGTM. Can you squash your commits?

allanrenucci · 2018-07-20T12:13:44Z

Thanks! 🎉

dottybot reviewed Jul 18, 2018

View reviewed changes

smarter reviewed Jul 18, 2018

View reviewed changes

allanrenucci reviewed Jul 19, 2018

View reviewed changes

allanrenucci approved these changes Jul 20, 2018

View reviewed changes

Fix #4404: In LambdaLift, modify flags properly

7b27b34

allanrenucci merged commit 0672b6d into scala:master Jul 20, 2018

Medowhill deleted the issue4404 branch July 20, 2018 12:43

Fix #4404: In LambdaLift, modify flags properly #4804

Fix #4404: In LambdaLift, modify flags properly #4804

Uh oh!

Conversation

Medowhill commented Jul 18, 2018

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter commented Jul 18, 2018

Uh oh!

Medowhill commented Jul 19, 2018

Uh oh!

allanrenucci commented Jul 19, 2018

Uh oh!

Medowhill commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allanrenucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Medowhill Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Jul 20, 2018

Uh oh!

Uh oh!

Medowhill commented Jul 19, 2018 •

edited

Loading

allanrenucci Jul 19, 2018 •

edited

Loading

Medowhill Jul 19, 2018 •

edited

Loading

allanrenucci left a comment •

edited

Loading